Cleanup tiles outside the texture - #77986

Hmm, sorry but I don't think this is the correct way to solve the bug. The whole API was meant to automatically remove tiles that are outside the texture (basically, if you reduce the texture size, tiles get deleted). You can have a look to TileSetAtlasSource::_clear_tiles_outside_texture that is called in set_texture

So the bug should likely be instead of fixing it that way.

This bug happens when a texture is updated externally. I thought that user might want to avoid data loss, hence the option.

I'll try to make it automatic. Still, there could be some corner cases where you might end up with invalid outside tiles and right now there is no way to remove them other than editing the tileset manually (which might be binary). I think the option is useful anyway.

I'll try to make it automatic. Still, there could be some corner cases where you might end up with invalid outside tiles and right now there is no way to remove them other than editing the tileset manually (which might be binary). I think the option is useful anyway.

I guess we should connect to the changed signal of Texture for that case. Theoretically it should work.

I'd rather not show the option if we can, a "fix bug" button isn't really the best IMO... But I guess, that, if this happens again for whatever reason, we should probably run this function internally from time to time (like when opening the atlas editor or something like that).

Ok I removed the option. The editor will do a sweep of invalid tiles when setting tileset + it connects to texture's changed signal.

Ah, that's was not what I really meant, sorry. Part of this should likely be done in the TileSetAtlasSource code itself, not in the editor. A bit like how it is already done to updated the padded texture:

void TileSetAtlasSource::set_texture(Ref<Texture2D> p_texture) {
	if (texture.is_valid()) {
		texture->disconnect(SNAME("changed"), callable_mp(this, &TileSetAtlasSource::_queue_update_padded_texture));
	}

	texture = p_texture;

	if (texture.is_valid()) {
		texture->connect(SNAME("changed"), callable_mp(this, &TileSetAtlasSource::_queue_update_padded_texture));
	}

	_clear_tiles_outside_texture();
	_queue_update_padded_texture();
	emit_changed();
}

That being said, it likely would make it quite difficult for the editor to save the tiles before the texture is reduced in size (for undo-redo). Which might be a big problem. I could manage to save tiles before deletion when the source itself was modified, but for sub-resources, I have no clue if that is possible.

So, in the end, I wonder if we should not modify the editor so that an atlas can have tiles outside of it's texture, and users would be able to clean them manually. But that's a lot of work...

I guess I can only see those solutions:

• accept the bug and add the button.
• Maybe find a way for the editor to detect the texture is about to be reduced in size, and save the tiles accordingly (I don't know how to implement this be we might be able to hack something)
• Allows tiles outside the texture: probably the cleanest but likely a lot of work.

Part of this should likely be done in the TileSetAtlasSource code itself, not in the editor.

But the tile cleanup is a costly operation and the problem is editor-only.

But the tile cleanup is a costly operation and the problem is editor-only.

Not really. If you modify the texture in game code, the tiles outside the texture will still be there, while they are not really supposed to. That's why the set_texture code itself I pasted above does include the call to _clear_tiles_outside_texture.
It's mostly visible in the editor, but the inconsistency is still there at runtime.

I am thinking that maybe a solution would be the following:

• Remove all calls to _clear_tiles_outside_texture() in code, then expose it as public method.
• Add an has_tile_outside_texture() public method
• Warn the user somehow in the editor, that the atlas has tiles outside the texture (by calling has_tile_outside_texture), and make it easy to click a button to cleanup the tiles outside the texture. A bit like you originally did. However, this should support undo/redo.
• Cleanup _undo_redo_inspector_callback in TileSetAtlasSourceEditor so that they don't save tiles outside the texture anymore, as it would not be needed. The get_tiles_to_be_removed_on_change function could likely be used before calling clear_tiles_outside_texture in the editor to save the tiles to be removed.

Done requested changes. The user is notified by the toaster:

I didn't do the _undo_redo_inspector_callback() changes as I didn't quite get what you meant.

cc @groud could you give it a review?

The message should not be spawned by the Toaster, but displayed as a warning sign with a tooltip somewhere in the atlas editor (maybe in the toolbar ?). Because the toaster messages will disappear after a little while.

Cleanup _undo_redo_inspector_callback in TileSetAtlasSourceEditor so that they don't save tiles outside the texture anymore, as it would not be needed. The get_tiles_to_be_removed_on_change function could likely be used before calling clear_tiles_outside_texture in the editor to save the tiles to be removed.

Part of the role of TileSetAtlasSourceEditor::_undo_redo_inspector_callback is to save things to avoid data loss in case of as value change. Without your PR, changing texture, margins, separation or region_size does cause some tiles to be removed automatically (with calls to _clear_tiles_outside_texture). TileSetAtlasSourceEditor::_undo_redo_inspector_callback simply saves the tile data of the deleted tiles so that using ctrl+z brings them back.

With your changes, _clear_tiles_outside_texture isn't called anymore. As a consequence, we can remove the logic saving the tiles data in TileSetAtlasSourceEditor::_undo_redo_inspector_callback. It should improve performance a bit by not saving useless data.

so whats up? will it merged?

@Isenther If it is approved. See the comment directly above yours.

The message should not be spawned by the Toaster, but displayed as a warning sign with a tooltip somewhere in the atlas editor (maybe in the toolbar ?). Because the toaster messages will disappear after a little while.

You mean the warning should be displayed permanently until you clear the tiles?

As a consequence, we can remove the logic saving the tiles data in TileSetAtlasSourceEditor::_undo_redo_inspector_callback

Not sure which code do you mean exactly.

Not sure which code do you mean exactly.

This can be removed:

	TileSetAtlasSourceProxyObject *atlas_source_proxy = Object::cast_to<TileSetAtlasSourceProxyObject>(p_edited);
	if (atlas_source_proxy) {
		Ref<TileSetAtlasSource> atlas_source = atlas_source_proxy->get_edited();
		ERR_FAIL_COND(!atlas_source.is_valid());
		UndoRedo *internal_undo_redo = undo_redo_man->get_history_for_object(atlas_source.ptr()).undo_redo;
		internal_undo_redo->start_force_keep_in_merge_ends();
		PackedVector2Array arr;
		if (p_property == "texture") {
			arr = atlas_source->get_tiles_to_be_removed_on_change(p_new_value, atlas_source->get_margins(), atlas_source->get_separation(), atlas_source->get_texture_region_size());
		} else if (p_property == "margins") {
			arr = atlas_source->get_tiles_to_be_removed_on_change(atlas_source->get_texture(), p_new_value, atlas_source->get_separation(), atlas_source->get_texture_region_size());
		} else if (p_property == "separation") {
			arr = atlas_source->get_tiles_to_be_removed_on_change(atlas_source->get_texture(), atlas_source->get_margins(), p_new_value, atlas_source->get_texture_region_size());
		} else if (p_property == "texture_region_size") {
			arr = atlas_source->get_tiles_to_be_removed_on_change(atlas_source->get_texture(), atlas_source->get_margins(), atlas_source->get_separation(), p_new_value);
		}
		if (!arr.is_empty()) {
			// Get all properties assigned to a tile.
			List<PropertyInfo> properties;
			atlas_source->get_property_list(&properties);
			for (int i = 0; i < arr.size(); i++) {
				Vector2i coords = arr[i];
				String prefix = vformat("%d:%d/", coords.x, coords.y);
				for (PropertyInfo pi : properties) {
					if (pi.name.begins_with(prefix)) {
						ADD_UNDO(atlas_source.ptr(), pi.name);
					}
				}
			}
		}
		internal_undo_redo->end_force_keep_in_merge_ends();
	}

What about my other question? >_>

You mean the warning should be displayed permanently until you clear the tiles?

Yes

I think this takes a bit too much space IMO. Sorry I was not clear, but my idea was to simply display a yellow warning icon in the toolbar, then use the tooltip to display the error message when hovering the icon.

At first I tried using the full text, but then moved half of it into the tooltip. Now it's actually small.
But yeah, icon makes sense.

Thanks!